feat(ipv6): full IPv6 support across the stack (#201) - #212

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

proxybroker/api.py (1)

382-388: 💤 Low value

Consider sharing the bracketed-IPv6 extraction with utils.find_proxy_pairs.

The inline loop here duplicates the v6 portion of find_proxy_pairs(text) (extract bracketed token → canonicalize_ip → drop on None). The reason _load cannot just call find_proxy_pairs directly is that find_proxy_pairs uses IPPortPatternGlobal (lookahead) for v4, while _load deliberately uses the line-anchored IPPortPatternLine (one pair per line — safer for raw-text and file inputs).

A small extraction keeps both call sites in sync as the v6 tokenizer evolves and matches the coding guideline that find_proxy_pairs is the single helper for [(ip_canonical, port), ...] output. As per coding guidelines: "use helper find_proxy_pairs(text) to return [(ip_canonical, port), ...] for both v4 and v6".

♻️ Suggested extraction

In proxybroker/utils.py, expose a v6-only helper alongside find_proxy_pairs:

def find_bracketed_ipv6_pairs(text):
    """Extract [(ip_canonical, port), ...] from bracketed IPv6:port forms."""
    pairs = []
    for raw_v6, port in IPv6BracketedPortPattern.findall(text):
        canonical = canonicalize_ip(raw_v6)
        if canonical is not None:
            pairs.append((canonical, port))
    return pairs

Then in api.py:

 from .utils import (
     IPPortPatternLine,
-    IPv6BracketedPortPattern,
-    canonicalize_ip,
+    find_bracketed_ipv6_pairs,
     log,
 )
@@
         if isinstance(data, str):
             v4_pairs = IPPortPatternLine.findall(data)
-            v6_pairs = []
-            for raw_v6, port in IPv6BracketedPortPattern.findall(data):
-                canonical = canonicalize_ip(raw_v6)
-                if canonical is not None:
-                    v6_pairs.append((canonical, port))
-            data = v4_pairs + v6_pairs
+            data = v4_pairs + find_bracketed_ipv6_pairs(data)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@proxybroker/api.py` around lines 382 - 388, Extract the bracketed-IPv6
parsing loop from _load and move it into a new helper like
find_bracketed_ipv6_pairs(text) in utils, which should use
IPv6BracketedPortPattern and canonicalize_ip to return [(ip_canonical, port),
...]; then in _load replace the inline loop with a call to that helper and
combine its result with IPPortPatternLine.findall(data) (i.e. v4_pairs +
find_bracketed_ipv6_pairs(data)); keep IPPortPatternLine,
IPv6BracketedPortPattern, canonicalize_ip and find_proxy_pairs behavior
unchanged.

proxybroker/utils.py (1)

41-47: 💤 Low value

IPv6BracketedPortPattern cannot capture zone IDs — minor consistency gap.

The character class [0-9A-Fa-f:.%]+ only allows hex digits, colon, dot, and percent. So a bracketed link-local with a zone ID like [fe80::1%eth0]:8080 will not match (t, h, etc. are outside [a-f]). The unbracketed _IPV6_CANDIDATE_PATTERN on line 27 does support zone IDs via (?:%[A-Za-z0-9_.\-]+)?, and canonicalize_ip's docstring (line 73) explicitly advertises preserving them, so the two paths disagree.

In practice, zone-IDs in published proxy feeds are rare, but if you want symmetry with the unbracketed tokenizer the inner class can be widened.

♻️ Optional widening for symmetry

-IPv6BracketedPortPattern = re.compile(r"\[([0-9A-Fa-f:.%]+)\]:(\d{2,5})")
+IPv6BracketedPortPattern = re.compile(
+    r"\[([0-9A-Fa-f:.]+(?:%[A-Za-z0-9_.\-]+)?)\]:(\d{2,5})"
+)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@proxybroker/utils.py` around lines 41 - 47, IPv6BracketedPortPattern
currently disallows zone ID characters, causing bracketed addresses like
[fe80::1%eth0]:8080 to not match; update the regex for IPv6BracketedPortPattern
so its inner capture mirrors the unbracketed _IPV6_CANDIDATE_PATTERN by allowing
zone ID characters after a percent (i.e., include A-Za-z0-9_.- or the equivalent
%... sequence inside the bracketed character class), ensuring canonicalize_ip's
promise to preserve zone IDs is honored for bracketed addresses too.

proxybroker/providers.py (1)

166-178: ⚡ Quick win

Consider using the find_proxy_pairs helper for consistency.

The manual extraction loop duplicates logic available in the find_proxy_pairs helper. As per coding guidelines, provider parsing should use find_proxy_pairs(text) which returns [(ip_canonical, port), ...] for both IPv4 and IPv6, already handling canonicalization and validation.

♻️ Refactor to use find_proxy_pairs helper

+from .utils import (
+    find_proxy_pairs,
+    get_headers,
+    log,
+)
+
 def _find_proxies(self, page):
-    # Default IPv4 extraction via the legacy pattern (subclasses may
-    # override `_pattern` for source-specific HTML quirks). IPv6
-    # entries in `[v6]:port` form are extracted in addition - they
-    # are validated and canonicalised via stdlib `ipaddress` so
-    # invalid bracketed garbage is silently dropped rather than
-    # surfacing as proxy candidates.
-    proxies = list(self._pattern.findall(page))
-    for raw_v6, port in IPv6BracketedPortPattern.findall(page):
-        canonical = canonicalize_ip(raw_v6)
-        if canonical is not None:
-            proxies.append((canonical, port))
-    return proxies
+    # Use find_proxy_pairs which handles both IPv4 and IPv6 extraction
+    # with canonicalization. Subclasses can still override this method
+    # for source-specific HTML quirks.
+    return find_proxy_pairs(page)

Based on coding guidelines: "Parse IPv6 provider feeds using IPv6BracketedPortPattern + stdlib validation, and use helper find_proxy_pairs(text) to return [(ip_canonical, port), ...] for both v4 and v6"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@proxybroker/providers.py` around lines 166 - 178, Replace the manual IPv6
extraction loop in _find_proxies with the shared helper: remove the
IPv6BracketedPortPattern loop and the extra canonicalize_ip logic and instead
return list(find_proxy_pairs(page)), ensuring you no longer rely on manual
_pattern.findall(page) + post-processing; use find_proxy_pairs(page) (which
returns [(ip_canonical, port), ...] for both IPv4 and IPv6) to keep
canonicalization/validation consistent across providers.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

tests/test_negotiators.py (1)

198-217: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

asyncio.get_event_loop().run_until_complete is deprecated and breaks on Python 3.14.

This pattern raises RuntimeError on Python 3.14 when no loop is running, so the test fails on the supported matrix. The other tests in this file already use @pytest.mark.asyncio + async def — adopt the same pattern here:

Fix

-    def test_socks4_ipv6_raises_helpful_error(self):
+    `@pytest.mark.asyncio`
+    async def test_socks4_ipv6_raises_helpful_error(self):
         """SOCKS4 RFC 1928 has no IPv6 address type. Asking for v6 over
         SOCKS4 should fail loudly, not silently truncate or crash with a
         cryptic struct error.
         """
-        # The current implementation crashes with OSError from inet_aton.
-        # We document the current observable behavior here so we notice
-        # if it ever changes silently. A future tighter check could raise
-        # a domain-specific BadResponseError instead.
-        import asyncio
         from unittest.mock import MagicMock

         from proxybroker.negotiators import Socks4Ngtr

         mock_proxy = MagicMock()
         ngtr = Socks4Ngtr(mock_proxy)
         with pytest.raises((OSError, ValueError)):
-            asyncio.get_event_loop().run_until_complete(
-                ngtr.negotiate(ip="2001:db8::1", port=443)
-            )
+            await ngtr.negotiate(ip="2001:db8::1", port=443)

As per coding guidelines: "Use async/await patterns consistently with modern asyncio; avoid deprecated asyncio patterns".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_negotiators.py` around lines 198 - 217, Replace the deprecated
asyncio.get_event_loop().run_until_complete usage in
test_socks4_ipv6_raises_helpful_error by making the test an async function
decorated with `@pytest.mark.asyncio` and directly await
ngtr.negotiate(ip="2001:db8::1", port=443); keep the same assertion that it
raises (OSError, ValueError) and retain the setup creating
Socks4Ngtr(mock_proxy) so the test logic and exception expectations remain
unchanged.

tests/test_resolver.py (1)

113-137: ⚡ Quick win

Avoid private-method coupling in fallback test.

This test currently patches resolver._resolve, which couples it to internal implementation instead of externally observable behavior. Prefer mocking the DNS query boundary and asserting the final resolved value.

Refactor direction

-    async def fake_resolve(host, qtype):
-        a_calls.append(qtype)
-        if qtype == "A":
-            raise ResolveError
-        from types import SimpleNamespace
-        return [SimpleNamespace(host="2001:db8::abcd")]
-
-    mocker.patch.object(resolver, "_resolve", side_effect=fake_resolve)
+    async def fake_query(_self, _host, qtype):
+        a_calls.append(qtype)
+        if qtype == "A":
+            raise asyncio.TimeoutError
+        from types import SimpleNamespace
+        return [SimpleNamespace(host="2001:db8::abcd")]
+
+    mocker.patch("aiodns.DNSResolver.query", side_effect=fake_query)

As per coding guidelines, tests/**/*.py: Test behavior and user-visible outcomes, not implementation details; avoid testing internal method calls or private attributes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_resolver.py` around lines 113 - 137, The test is coupling to the
private method resolver._resolve; change it to mock the external DNS query
boundary that resolver.resolve uses (for example the aiodns.DNSResolver.query or
the module/function that performs the actual DNS lookup in the resolver
implementation) so you only assert externally observable behavior: call
resolver.resolve("v6only.example.com") and assert it returns "2001:db8::abcd"
while removing assertions about a_calls or explicit checks of resolver._resolve;
keep the fake response to return an aiodns-style record
(SimpleNamespace(host="2001:db8::abcd")) for the AAAA query so the fallback path
is exercised without touching private methods.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

proxybroker/resolver.py (1)

164-191: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Canonicalize resolve() outputs before returning or caching them.

Direct IP inputs and DNS answers still leave resolve() as raw text, so callers can get non-RFC-5952 variants even though the rest of the stack now compares canonicalized IPs. Run both the early-return path and each r.host through canonicalize_ip(...) before storing or returning them.

Suggested fix

         if self.host_is_ip(host):
-            return host
+            return canonicalize_ip(host) or host
@@
         if resp:
             hosts = [
                 {
                     "hostname": host,
-                    "host": r.host,
+                    "host": canonicalize_ip(r.host) or r.host,
                     "port": port,
                     "family": family,
                     "proto": socket.IPPROTO_IP,
                     "flags": socket.AI_NUMERICHOST,
                 }

As per coding guidelines, **/*.py: Canonicalize IPv6 addresses using RFC 5952 format at every text-to-comparison boundary, and use proxybroker.utils.canonicalize_ip(s) -> str | None when IPs cross from text into comparison contexts.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@proxybroker/resolver.py` around lines 164 - 191, The resolve() implementation
must canonicalize IP text before returning or caching: when host_is_ip(host)
returns True, pass host through proxybroker.utils.canonicalize_ip(host) and
return that canonical value (or None-handled), and when building hosts from resp
(from _race_a_aaaa or _resolve) run each r.host through canonicalize_ip before
placing it into the hosts entries and before storing into self._cached_hosts;
ensure you handle canonicalize_ip returning None (skip or fallback to original)
and keep caching logic for family vs single-host consistent with the existing
_cached_hosts usage.

proxybroker/negotiators.py (1)

93-96: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Parse the SOCKS5 reply from the response ATYP, not the request ATYP.

reply_size = 22 if atyp == 0x04 else 10 assumes the proxy binds with the same address family the client requested. That is not guaranteed, so a dual-stack proxy can legally return a shorter or longer BND.ADDR here. In that case this read will under/over-read the socket and can stall the negotiation.

Suggested fix

         await self._proxy.send(request)
-        # Reply size: VER(1)+REP(1)+RSV(1)+ATYP(1)+BND.ADDR(4 or 16)+BND.PORT(2)
-        reply_size = 22 if atyp == 0x04 else 10
-        resp = await self._proxy.recv(reply_size)
-
-        if resp[0] != 0x05 or resp[1] != 0x00:
+        header = await self._proxy.recv(4)
+        if header[0] != 0x05 or header[1] != 0x00:
             self._proxy.log("Failed (invalid data)", err=BadResponseError)
             raise BadResponseError
-        else:
-            self._proxy.log("Request is granted")
+        rep_atyp = header[3]
+        if rep_atyp == 0x01:
+            addr_len = 4
+        elif rep_atyp == 0x04:
+            addr_len = 16
+        elif rep_atyp == 0x03:
+            length = await self._proxy.recv(1)
+            addr_len = length[0]
+        else:
+            self._proxy.log("Failed (invalid data)", err=BadResponseError)
+            raise BadResponseError
+
+        await self._proxy.recv(addr_len + 2)
+        self._proxy.log("Request is granted")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@proxybroker/negotiators.py` around lines 93 - 96, The code currently uses the
request variable atyp to compute reply_size and does a single recv that can
under/over-read; change the logic in the SOCKS5 negotiation (around
self._proxy.send and self._proxy.recv) to first read the fixed 4-byte reply
header (VER, REP, RSV, ATYP) from the proxy, extract the response ATYP (not the
request atyp), then compute the remaining payload length (BND.ADDR length = 4
for IPv4, 16 for IPv6, or variable for domain) and do a second recv for the rest
of the reply; update variable names (e.g., resp_header vs resp_body) and ensure
you use the response ATYP to determine reply_size rather than the original atyp.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

tests/test_resolver.py (2)

63-66: ⚡ Quick win

This test currently mocks the method it claims to validate.

test_get_real_ext_ip patches resolver.get_real_ext_ip and then asserts the patched value, so it can’t catch regressions in real behavior. Consider removing it or replacing it with an observable contract case (like the canonicalization test below).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_resolver.py` around lines 63 - 66, The test test_get_real_ext_ip
incorrectly patches the very function under test (resolver.get_real_ext_ip), so
it never validates real behavior; remove the patch and either call the real
async resolver.get_real_ext_ip (optionally with a small aiohttp-responses or
requests-mocking fixture) to assert the actual IP/canonicalization behavior, or
replace the test with a contract-style check that exercises canonicalization
logic (e.g., feed a known external-response into the resolver's HTTP client and
assert the normalized IP result). Locate test_get_real_ext_ip and
resolver.get_real_ext_ip to implement the change and ensure the test validates
real logic rather than a mocked return.

---

125-330: 🏗️ Heavy lift

Reduce coupling to private resolver internals in race/cache tests.

A lot of these tests assert _resolve call counts and mutate _cached_hosts directly. That overfits to internals and makes refactors brittle. Prefer asserting externally visible outcomes (resolved value/error behavior) with collaborator fakes instead.

As per coding guidelines, **/test_*.py: Test behavior, not implementation; focus on user-visible outcomes and don't test internal method calls or private attributes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_resolver.py` around lines 125 - 330, The tests currently couple to
private internals by clearing resolver._cached_hosts and asserting
resolver._resolve.call_count; change them to exercise only the public API: stop
mutating _cached_hosts directly and instead create a fresh resolver fixture or
use whatever public cache-clear method exists (or re-instantiate resolver)
before each case, remove assertions that check resolver._resolve.call_count and
any spy on _resolve, and assert the externally visible results from
resolver.resolve (returned address lists, exceptions, and cached-return behavior
via repeated resolver.resolve calls) while keeping the existing
aiodns.DNSResolver.query fakes to control DNS outcomes.

tests/test_negotiators.py (1)

100-137: 🏗️ Heavy lift

Avoid asserting private helper _CONNECT_request directly.

These tests are tightly coupled to a private function, which makes refactors noisy. Prefer driving the same assertions through public negotiator contracts (e.g., via NGTRS[...] + sent request bytes).

As per coding guidelines, **/test_*.py: Test behavior, not implementation; focus on user-visible outcomes and don't test internal method calls or private attributes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_negotiators.py` around lines 100 - 137, The tests call the private
helper _CONNECT_request directly; instead, drive the same assertions through the
public negotiator contract by using the NGTRS mapping to obtain the appropriate
negotiator class, instantiate it (or use its public API) and generate the
outgoing request bytes via the negotiator's public send/build method, then
assert the Host/authority and bracket behavior on those bytes; remove direct
references to _CONNECT_request and replace with calls to NGTRS[...] (the public
negotiator) to make the tests validate observable behavior rather than the
private helper.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes